Add lint target to makefile#51
Merged
Merged
Conversation
|
Important Review skippedToo many files! This PR contains 298 files, which is 148 over the limit of 150. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (298)
You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughConsolidates linting into a Makefile-driven flow and tooling module, updates golangci-lint configuration and formatters, renames several packages from snake_case to camelCase, adds error checks when registering informer handlers, and adjusts startup/shutdown ordering in main.go. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
main.go (1)
66-69: Consider handling SIGINT for local development.Currently only
SIGTERMis handled. For easier local development/debugging, you may want to also handleSIGINT(Ctrl+C). This is optional since container orchestrators typically useSIGTERM.♻️ Optional: Add SIGINT handling
signalCh := make(chan os.Signal, 1) - signal.Notify(signalCh, syscall.SIGTERM) + signal.Notify(signalCh, syscall.SIGTERM, syscall.SIGINT) <-signalCh🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.go` around lines 66 - 69, The shutdown handling only listens for SIGTERM on signalCh; update the signal.Notify call to also listen for SIGINT so local Ctrl+C triggers graceful shutdown—for example modify the call referencing signalCh and signal.Notify to include syscall.SIGINT alongside syscall.SIGTERM (and adjust the comment to note SIGINT is for local development). Ensure the rest of the code that reads from signalCh (the <-signalCh line) remains unchanged.pkg/routes-handler/routes-handler.go (1)
21-21: Same pattern as node-port-handler: consider failing fast on registration error.This has the same concern as
NodePortHandler- the informer runs without event handlers if registration fails. Consider propagating the error to the caller for consistent behavior.Also applies to: 50-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/routes-handler/routes-handler.go` at line 21, The informer event-handler registration (informer.AddEventHandler with cache.ResourceEventHandlerFuncs) currently ignores registration failures; change the code to check the returned error and propagate it to the caller so the informer doesn't run without handlers. Specifically, after calling informer.AddEventHandler(...) (and the other similar calls around the second occurrence), if err != nil return that error (or wrap it with context) from the enclosing function instead of discarding it; ensure the enclosing function's signature allows returning an error so failures are surfaced consistently.pkg/node-port-handler/node-port-handler.go (1)
26-26: Consider failing fast if event handler registration fails.The error is logged but the informer is still returned and will be run. If
AddEventHandlerfails, the informer will run without processing any events, silently doing nothing useful. Consider returning an error or usinglog.Fatalto prevent silent failure.♻️ Suggested approach: return error to caller
-func NodePortHandler(clientset *kubernetes.Clientset) cache.SharedIndexInformer { +func NodePortHandler(clientset *kubernetes.Clientset) (cache.SharedIndexInformer, error) { factory := informers.NewSharedInformerFactory(clientset, 5*time.Minute) informer := factory.Core().V1().Services().Informer() _, err := informer.AddEventHandler(cache.ResourceEventHandlerFuncs{ // ... handlers ... }) if err != nil { - log.Errorf("failed to add event handler: %v", err) + return nil, fmt.Errorf("failed to add event handler: %w", err) } - return informer + return informer, nil }Then update
main.goto handle the error appropriately.Also applies to: 80-82
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/node-port-handler/node-port-handler.go` at line 26, The informer registration currently swallows AddEventHandler errors (in the call to informer.AddEventHandler with cache.ResourceEventHandlerFuncs) and only logs them, so the informer may run without handlers; change the code to fail fast by returning an error from the function when informer.AddEventHandler returns a non-nil err (i.e., update the function signature to return (informerType, error) or appropriate value+error, propagate the error instead of logging and returning the informer), and update callers (e.g., main.go) to handle the returned error; apply the same change for the other AddEventHandler occurrence that currently logs but continues.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Around line 21-24: The Makefile currently checks only $(shell go env
GOPATH)/bin for golangci-lint and installs with `@latest`, which is
non-deterministic and ignores GOBIN; update the lint target to resolve the
binary using $(shell go env GOBIN) if set else $(shell go env GOPATH)/bin (or
use a small helper variable like GOBIN_FALLBACK) and use a pinned version
variable (e.g., GOLANGCI_LINT_VERSION) instead of `@latest` when invoking go
install; ensure the target references that resolved path when both checking
existence and running golangci-lint so the install and execution are GOBIN-aware
and CI-reproducible.
---
Nitpick comments:
In `@main.go`:
- Around line 66-69: The shutdown handling only listens for SIGTERM on signalCh;
update the signal.Notify call to also listen for SIGINT so local Ctrl+C triggers
graceful shutdown—for example modify the call referencing signalCh and
signal.Notify to include syscall.SIGINT alongside syscall.SIGTERM (and adjust
the comment to note SIGINT is for local development). Ensure the rest of the
code that reads from signalCh (the <-signalCh line) remains unchanged.
In `@pkg/node-port-handler/node-port-handler.go`:
- Line 26: The informer registration currently swallows AddEventHandler errors
(in the call to informer.AddEventHandler with cache.ResourceEventHandlerFuncs)
and only logs them, so the informer may run without handlers; change the code to
fail fast by returning an error from the function when informer.AddEventHandler
returns a non-nil err (i.e., update the function signature to return
(informerType, error) or appropriate value+error, propagate the error instead of
logging and returning the informer), and update callers (e.g., main.go) to
handle the returned error; apply the same change for the other AddEventHandler
occurrence that currently logs but continues.
In `@pkg/routes-handler/routes-handler.go`:
- Line 21: The informer event-handler registration (informer.AddEventHandler
with cache.ResourceEventHandlerFuncs) currently ignores registration failures;
change the code to check the returned error and propagate it to the caller so
the informer doesn't run without handlers. Specifically, after calling
informer.AddEventHandler(...) (and the other similar calls around the second
occurrence), if err != nil return that error (or wrap it with context) from the
enclosing function instead of discarding it; ensure the enclosing function's
signature allows returning an error so failures are surfaced consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 34cee140-3697-4b13-a41e-a1d2c2396e7b
📒 Files selected for processing (7)
.github/workflows/ci.yml.golangci.ymlMakefilemain.gopkg/node-port-handler/node-port-handler.gopkg/routes-handler/routes-handler.gopkg/routes-handler/routes-handler_test.go
cfergeau
reviewed
Mar 31, 2026
cfergeau
reviewed
Mar 31, 2026
| @@ -1,5 +1,11 @@ | |||
| TAG ?= $(shell git describe --match=NeVeRmAtCh --always --abbrev=40 --dirty) | |||
| CONTAINER_RUNTIME ?= podman | |||
| GOLANGCI_LINT_VERSION ?= v1.62.2 | |||
Contributor
There was a problem hiding this comment.
with something like crc-org/vfkit@3cc650a ,
- golangci-lint with a hardcoded version does not get installed in GOBIN, potentially conflicting with the version needed by other modules
- dependabot updates golangci-lint for us
Member
Author
There was a problem hiding this comment.
yes, I am trying out how good claude is for this task.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/tools.mk`:
- Line 1: The TOOLS_BINDIR assignment uses realpath which returns empty for
non-existent paths; change the assignment to use abspath (i.e. TOOLS_BINDIR :=
$(abspath $(TOOLS_DIR)/bin)) so the bin path is correct on a fresh clone, and
update the install/lint recipe(s) that rely on TOOLS_BINDIR (and GOBIN) to
create the directory first (mkdir -p $(TOOLS_BINDIR)) before installing
golangci-lint or setting GOBIN so installations go into the intended tools/bin
location.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a4ca8e58-a814-4d8b-8605-6d888e677b68
⛔ Files ignored due to path filters (1)
tools/go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
.github/workflows/ci.yml.golangci.ymlMakefilemain.gopkg/node-port-handler/node-port-handler.gopkg/routes-handler/routes-handler.gopkg/routes-handler/routes-handler_test.gotools/bin/.gitignoretools/go.modtools/tools.gotools/tools.mk
✅ Files skipped from review due to trivial changes (4)
- tools/bin/.gitignore
- tools/tools.go
- .github/workflows/ci.yml
- pkg/routes-handler/routes-handler_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- Makefile
- pkg/routes-handler/routes-handler.go
- .golangci.yml
- pkg/node-port-handler/node-port-handler.go
- main.go
cfergeau
reviewed
Apr 28, 2026
| makefat | ||
| mockery | ||
| goimports | ||
| goimports.exe |
Contributor
There was a problem hiding this comment.
We will only be building golangci-lint, not the other ones. This could be * or such instead of naming the individual binaries.
cfergeau
reviewed
Apr 28, 2026
cfergeau
left a comment
cfergeau
left a comment
Contributor
There was a problem hiding this comment.
A few minor comments, but overall /lgtm
| - gofmt | ||
| - goimports | ||
| exclusions: | ||
| generated: lax |
Contributor
There was a problem hiding this comment.
Is this coming from another project? Maybe we can have stricter rules than the project this came from?
Contributor
I’ve pushed a
|
cfergeau
approved these changes
Jun 23, 2026
- Install golangci-lint if not available - Fix all the lint error - Update the ci job
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Chores